Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove most box syntax from librustdoc #99066

Merged
merged 7 commits into from
Jul 13, 2022
Merged

Remove most box syntax from librustdoc #99066

merged 7 commits into from
Jul 13, 2022

Conversation

est31
Copy link
Member

@est31 est31 commented Jul 8, 2022 •
edited
Loading

This is the second attempt after the librustdoc specific changes have been reverted from #87781 in #89134, due to a minor, but exant regression caused by the changes. There have been some changes to librustdoc in the past and maybe thanks to them there is no regression any more. If there is still a regression, one can investigate further and maybe find ways to fix the regressions. Thus, i request a perf run. Edit: turns out there is still a regression, but it's caused only by a subset of the changes. So I've changed this PR to only contains the changes that don't cause any performance regressions, keeping the regression causing changes for a later PR.

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jul 8, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 8, 2022

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 8, 2022
@compiler-errors
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 8, 2022
@bors
Copy link
Contributor

bors commented Jul 8, 2022

⌛ Trying commit 5e1e89e5d4b6c9b3fa076a17aa5455471beb477c with merge 567cf89ba17f13bc490de32582f36c7ed9c07c62...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jul 8, 2022

☀️ Try build successful - checks-actions
Build commit: 567cf89ba17f13bc490de32582f36c7ed9c07c62 (567cf89ba17f13bc490de32582f36c7ed9c07c62)

@rust-timer
Copy link
Collaborator

Queued 567cf89ba17f13bc490de32582f36c7ed9c07c62 with parent 45263fc, future comparison URL.

@jyn514
Copy link
Member

jyn514 commented Jul 8, 2022

I'm no longer on the rustdoc team, please don't assign me on PRs.

r? rust-lang/rustdoc

@rust-highfive rust-highfive assigned jsha and unassigned jyn514 Jul 8, 2022
@est31
Copy link
Member Author

est31 commented Jul 8, 2022

I'm no longer on the rustdoc team, please don't assign me on PRs.

@jyn514 didn't know that, sorry!

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (567cf89ba17f13bc490de32582f36c7ed9c07c62): comparison url.

Instruction count

  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)		 0.3% 0.4% 7
Regressions 😿
(secondary)		 1.0% 2.0% 14
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 0.3% 0.4% 7

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 4.4% 5.8% 2
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)		 5.8% 5.8% 1
Regressions 😿
(secondary)		 3.4% 4.5% 7
Improvements 🎉
(primary)		 -2.0% -2.0% 1
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 1.9% 5.8% 2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 9, 2022
@est31
Remove box syntax from Box<function pointer> construction
94a0923 
The function pointer should be extremely small, close to pointer size.
@est31
Remove box syntax from doctest.rs
cb5e94b 
Doctests are fairly cold code, so even if there
is a regression, which i doubt, it won't matter.
@est31
Copy link
Member Author

est31 commented Jul 10, 2022

Okay so the regression is still around. I've split up the change into multiple commits. I've sorted them by how much performance-relevant I think they are, so mostly by size if the type is obvious. This is of course not perfect, e.g. cold code should be less performance relevant than hot code, and what also matters is which optimizations LLVM is doing. But I'm doing a first approximation for now.

I was surprised by how large ItemKind was: 240 bytes! It might be an idea to work on reducing the size by additional boxing. Maybe there are already issues/PRs for it? Need to check. For now I have excluded structs larger than 100 bytes from the PR, so ItemKind is not included.

Can this get another perf run? Then I'll add back the larger structs and will ask for another perf run (because it's an effect close to the noise level).

@jyn514
Copy link
Member

jyn514 commented Jul 10, 2022

I was surprised by how large ItemKind was: 240 bytes! It might be an idea to work on reducing the size by additional boxing. Maybe there are already issues/PRs for it? Need to check. For now I have excluded structs larger than 100 bytes from the PR, so ItemKind is not included.

#79975

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@est31
Remove box syntax from Box<dyn Iterator> construction
1ac17fc 
The iterators created should be pretty light weight.
@est31
Remove box syntax for Box<Attributes> construction
ccf1bdb 
Attributes only has 48 bytes according to compiler internal rustdoc.
@est31
Remove box syntax for Box<ImplItem> construction
88d72a9 
ImplItem only has 80 bytes according to compiler internal rustdoc.
@est31
Remove box syntax for Box<rustdoc::clean::types::Type> construction
3fa637d 
The type has 80 bytes according to compiler internal rustdoc.
@est31 est31 changed the title Remove box syntax from librustdoc Remove most box syntax from librustdoc Jul 12, 2022
@bors
Copy link
Contributor

bors commented Jul 12, 2022

☀️ Try build successful - checks-actions
Build commit: 3be870053e75dea2d9bcebccada54f6cc32df79d (3be870053e75dea2d9bcebccada54f6cc32df79d)

@rust-timer
Copy link
Collaborator

Queued 3be870053e75dea2d9bcebccada54f6cc32df79d with parent b3f4c31, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3be870053e75dea2d9bcebccada54f6cc32df79d): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 -2.8% -2.8% 1
Improvements 🎉
(secondary)		 -2.8% -2.8% 1
All 😿🎉 (primary) -2.8% -2.8% 1

Cycles

This benchmark run did not return any relevant results for this metric.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩

  2. number of relevant changes ↩

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 12, 2022
@est31
Copy link
Member Author

est31 commented Jul 12, 2022

Yes, so it's the ItemKind changes only. Ready for review.

@jsha
Copy link
Contributor

jsha commented Jul 13, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Jul 13, 2022

📌 Commit 3d2494d has been approved by jsha

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 13, 2022
@bors
Copy link
Contributor

bors commented Jul 13, 2022

⌛ Testing commit 3d2494d with merge 75b4af84d83713ae1497e54a971e82c0e004e53d...

@bors
Copy link
Contributor

bors commented Jul 13, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 13, 2022
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
.......... (60/66)
.....     (66/66)


/checkout/src/test/rustdoc-gui/search-result-display.goml search-result-display... FAILED
[ERROR] (line 5) Error: The following CSS selector "#search-settings" was not found: for command `wait-for: "#search-settings"`
Build completed unsuccessfully in 0:00:46

@jyn514
Copy link
Member

jyn514 commented Jul 13, 2022

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 13, 2022
@bors
Copy link
Contributor

bors commented Jul 13, 2022

⌛ Testing commit 3d2494d with merge a639f89...

@bors
Copy link
Contributor

bors commented Jul 13, 2022

☀️ Test successful - checks-actions
Approved by: jsha
Pushing a639f89 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 13, 2022
@bors bors merged commit a639f89 into rust-lang:master Jul 13, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 13, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a639f89): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -3.5% -3.5% 1
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 2.9% 3.8% 2
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2

  2. number of relevant changes ↩ ↩2

@est31 est31 mentioned this pull request Jul 21, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 29, 2022
@bors
Auto merge of rust-lang#99577 - est31:remove_box_librustdoc, r=jsha
3924dac 
Remove remaining uses of box syntax from librustdoc

Remove the remaining uses of box syntax from librustdoc. Followup of rust-lang#99066 where these changes were split out because they were responsible for a small but noticeable regression. This PR avoids the regression by boxing some large variants of `ItemKind` to reduce the enum's size by half from 224 bytes to 112 bytes (on x86-64). This should also help with reducing memory usage.
@est31 est31 mentioned this pull request Feb 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jsha jsha

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.64.0
Development

Successfully merging this pull request may close these issues.